-
Notifications
You must be signed in to change notification settings - Fork 439
impl(generator): emit ud samples and ud dox md files #15654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the code generator's capabilities by enabling it to automatically create C++ code samples and comprehensive Doxygen documentation for overriding the universe domain in generated client libraries. This enhancement ensures that users have clear, readily available guidance on how to configure their client applications for various Google Cloud environments, improving usability and consistency across libraries. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the client libraries by introducing new C++ samples and corresponding documentation for overriding the default universe domain. This change streamlines the process for developers to configure universe domains, improving clarity and ease of use when working with different Google Cloud environments. The updates involve modifications to the code generation logic and documentation scripts to ensure these samples are consistently generated and integrated across various services. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the code generator to automatically produce C++ samples demonstrating how to override the universe domain for various client libraries. This includes generating the necessary code snippets and integrating them into the Doxygen documentation, thereby improving the clarity and usability of this advanced configuration option for developers. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the generation of samples and documentation for overriding the universe domain. The changes to the generator scripts and templates look good. However, I found a critical issue in the sample generator that produces uncompilable code. The generated code for setting the universe domain is missing a variable declaration and uses a StatusOr object where an Options object is expected. My review includes a suggestion to fix this in the generator, which will correct all the generated sample files.
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | ||
| if (!ud_options.ok()) throw std::move(ud_options).status(); | ||
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | ||
| google::cloud::$product_namespace$::Make$connection_class_name$(ud_options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generated code for SetClientUniverseDomain has a few issues that will prevent it from compiling and may cause warnings:
- The
ud_optionsvariable is used without being declared. It should be declared withauto. google::cloud::AddUniverseDomainOptionreturns agoogle::cloud::StatusOr<Options>. TheMake...Connectionfunction expects agoogle::cloud::Optionsobject, but it's being passed theStatusOrobject directly. TheStatusOrobject needs to be checked for an error and then dereferenced using the*operator.- The check
!ud_options.ok()is valid, but using!ud_optionsis more idiomatic forStatusOrobjects. - The
ud_clientvariable is not used, which will result in a compiler warning. This can be suppressed by casting it tovoid.
Here is a corrected version of the generated code:
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | |
| if (!ud_options.ok()) throw std::move(ud_options).status(); | |
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | |
| google::cloud::$product_namespace$::Make$connection_class_name$(ud_options)); | |
| auto ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | |
| if (!ud_options) throw std::move(ud_options).status(); | |
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | |
| google::cloud::$product_namespace$::Make$connection_class_name$(*ud_options)); | |
| (void)ud_client; |
diegomarquezp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one minor question.
| // in the provided Options for the Universe Domain associated with the | ||
| // credentials and adds it to the set of Options. | ||
| // If no UnifiedCredentialsOption is set, GoogleDefaultCredentials are used. | ||
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ud_options assumed to be present elsewhere in the generated samples? I'm asking cause I would expect a local declaration with auto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got ahead of myself and though I had verified this sample as working code. I had not.
Yes, ud_options does need to be declared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the capability to generate C++ samples and documentation for overriding the universe domain. The changes to the generator scripts and templates are mostly correct and follow existing patterns. However, I've found a critical issue in the sample generator that produces non-compiling code. The ud_options variable is used before being declared, and the StatusOr object is passed to a function expecting the contained Options value. My review includes a specific comment with a code suggestion to fix this. Once the generator is corrected, all the generated sample files should be fixed.
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | ||
| if (!ud_options.ok()) throw std::move(ud_options).status(); | ||
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | ||
| google::cloud::$product_namespace$::Make$connection_class_name$(ud_options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generated code has a couple of issues that will prevent it from compiling:
- The
ud_optionsvariable is used without being declared. It should be declared withauto. - The
ud_optionsvariable is agoogle::cloud::StatusOr<google::cloud::Options>, but theMake$connection_class_name$function expects agoogle::cloud::Optionsobject. You need to dereferenceud_options(e.g., using*ud_options) to pass the contained value.
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | |
| if (!ud_options.ok()) throw std::move(ud_options).status(); | |
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | |
| google::cloud::$product_namespace$::Make$connection_class_name$(ud_options)); | |
| auto ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | |
| if (!ud_options.ok()) throw std::move(ud_options).status(); | |
| auto ud_client = google::cloud::$product_namespace$::$client_class_name$( | |
| google::cloud::$product_namespace$::Make$connection_class_name$(*ud_options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds the capability to generate C++ samples and documentation for overriding the universe domain. The changes correctly modify the generator to produce the necessary sample code and Doxygen files. The implementation includes refactoring the sample generator for better modularity. However, there is a critical issue in the generated sample code where a variable is used without being declared, which will cause compilation failures. This needs to be addressed.
| // in the provided Options for the Universe Domain associated with the | ||
| // credentials and adds it to the set of Options. | ||
| // If no UnifiedCredentialsOption is set, GoogleDefaultCredentials are used. | ||
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable ud_options is used without being declared. This will cause a compilation error in all generated sample files for this feature. It should be declared using auto to capture the StatusOr<Options> returned by AddUniverseDomainOption.
| ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); | |
| auto ud_options = google::cloud::AddUniverseDomainOption(std::move(options)); |
2df9f92 to
4aa572e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15654 +/- ##
========================================
Coverage 93.10% 93.10%
========================================
Files 2439 2439
Lines 223941 224070 +129
========================================
+ Hits 208493 208615 +122
- Misses 15448 15455 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR modifies the generator produce C++ samples for overriding the universe domain, as well as, emitting dox files that incorporate the samples to produce the desired documentation.
The first commit has all the hand written changes. The other commits are outputs of the generation process.